Skip to content

[improve][broker] PIP-433: Ensure topic creation before starting GEO #24652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
nodece wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[improve][broker] PIP-433: Ensure topic creation before starting GEO #24652

nodece wants to merge 6 commits into from
+374 −54

Conversation

@nodece
Copy link
Member

@nodece nodece commented Aug 20, 2025 •
edited
Loading

PIP: #24485

Motivation

In a GEO replication scenario, if the remote cluster does not have the replicated topic and the auto-creation type differs between the local and remote clusters, message replication may fail. To ensure seamless replication, the topic metadata must be properly synchronized across clusters.

This is part of PIP-433.

#24136 is the same as this PR.

Modifications

  • When both the local and remote partitioned topic metadata indicate partitions=0, this means the topic is non-partitioned. In this case, the local cluster sends a non-partitioned topic creation request to the remote cluster.

  • If the local partitioned topic metadata has partitions>0, this means the topic is partitioned:

    • If the remote partitioned topic metadata has partitions=0, the local cluster sends a partitioned topic creation request to the remote cluster.
    • If partitions differ between the local and remote cluster, please stop GEO.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 20, 2025
@nodece nodece force-pushed the improve-topic-creation-and-partition-update-before-starting-geo branch from 9f91e51 to a613da2 Compare August 20, 2025 14:28
@poorbarcode poorbarcode added this to the 4.2.0 milestone Sep 3, 2025
poorbarcode
poorbarcode reviewed Sep 3, 2025
View reviewed changes
Copy link
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodece Thanks for driving this feature

...roker/src/main/java/org/apache/pulsar/broker/service/persistent/GeoPersistentReplicator.java Outdated
}
// Set useFallbackForNonPIP344Brokers to true when mix of PIP-344 and non-PIP-344 brokers are used, it
// can still work.
return client.getLookup().getPartitionedTopicMetadata(baseTopicName, false, true)
Copy link
Contributor

@poorbarcode poorbarcode Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have pulsarAdmin object, can we use pulsar-admin.topics.getPartitionedTopicMetadataAsync here?

Copy link
Member Author

@nodece nodece Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will refactor this logic.

Copy link
Member Author

@nodece nodece Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poorbarcode This is an asynchronous method, but the keyword async is missing from its name.

Copy link
Contributor

@poorbarcode poorbarcode Nov 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have pulsarAdmin object, can we use pulsar-admin.topics.getPartitionedTopicMetadataAsync here?

I have modified the code, please review it. f7d38d7

Copy link
Member Author

@nodece nodece Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poorbarcode Good, I should use pulsarAdmin instead of pulsarClient, this is better way.

@poorbarcode
Copy link
Contributor

poorbarcode commented Nov 24, 2025

Rebase master

@nodece nodece force-pushed the improve-topic-creation-and-partition-update-before-starting-geo branch from a47eb1f to 4c833ed Compare November 25, 2025 03:34
@poorbarcode poorbarcode closed this Dec 1, 2025
@poorbarcode poorbarcode reopened this Dec 1, 2025
@nodece nodece force-pushed the improve-topic-creation-and-partition-update-before-starting-geo branch from 90de7bd to 36661e4 Compare December 25, 2025 08:59
@nodece
Copy link
Member Author

nodece commented Dec 25, 2025 •
edited
Loading

The tests testReplicatorWithSpecialNonPartitionedTopic, testFailureReplicatorWithSpecialNonPartitionedTopic, and testFailureReplicatorWithSpecialNonPartitionedTopic2 were removed in commit 4238cbd. This behavior is no longer applicable because the corresponding scenario introduced by PIP-414 has been disabled, and PIP-433 was not cherry-picked into the older branch.

The new code has been refactored by the AI.

@nodece
Copy link
Member Author

nodece commented Dec 25, 2025

ping @poorbarcode

@poorbarcode
Copy link
Contributor

poorbarcode commented Dec 25, 2025

@nodece I will review the PR again by this weekend

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 39.65517% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.81%. Comparing base (ab65faa) to head (36661e4).
⚠️ Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
...er/service/persistent/GeoPersistentReplicator.java 30.23% 27 Missing and 3 partials ⚠️
...sar/broker/service/persistent/PersistentTopic.java 44.44% 4 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (ab65faa) and HEAD (36661e4). Click for more details.

HEAD has 23 uploads less than BASE
Flag BASE (ab65faa) HEAD (36661e4)
unittests 12 1
inttests 8 2
systests 8 2
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #24652       +/-   ##
=============================================
- Coverage     74.75%   38.81%   -35.95%     
+ Complexity    33794    13232    -20562     
=============================================
  Files          1899     1842       -57     
  Lines        149656   145523     -4133     
  Branches      17393    16914      -479     
=============================================
- Hits         111877    56479    -55398     
- Misses        28978    81316    +52338     
+ Partials       8801     7728     -1073
Flag Coverage Δ
inttests 26.66% <39.65%> (-0.12%) ⬇️
systests 23.05% <0.00%> (-0.15%) ⬇️
unittests 34.61% <0.00%> (-39.67%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ache/pulsar/broker/service/AbstractReplicator.java 31.66% <100.00%> (-35.99%) ⬇️
...service/nonpersistent/NonPersistentReplicator.java 57.54% <ø> (-6.61%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 53.55% <100.00%> (-19.34%) ⬇️
...roker/service/persistent/PersistentReplicator.java 32.57% <ø> (-41.01%) ⬇️
...ar/broker/service/persistent/ShadowReplicator.java 0.00% <ø> (-45.91%) ⬇️
...sar/broker/service/persistent/PersistentTopic.java 50.66% <44.44%> (-29.73%) ⬇️
...er/service/persistent/GeoPersistentReplicator.java 32.46% <30.23%> (-39.19%) ⬇️

... and 1402 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodece nodece force-pushed the improve-topic-creation-and-partition-update-before-starting-geo branch from 36661e4 to f2e4b5c Compare January 8, 2026 02:06
@nodece nodece force-pushed the improve-topic-creation-and-partition-update-before-starting-geo branch from 62df37d to b5953d9 Compare January 9, 2026 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@poorbarcode poorbarcode poorbarcode approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@dao-jun dao-jun Awaiting requested review from dao-jun

Assignees

@nodece nodece

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

4.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nodece @poorbarcode @codecov-commenter